Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: infinite approval doesn't persist after transfer #217

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

danielattilasimon
Copy link
Collaborator

@danielattilasimon danielattilasimon commented Jun 18, 2024

This fixes a know quirk of LUSD/BOLD, wherein an infinite approval is "only" practically infinite, but actually decreases upon each transfer. This made it trickier to detect infinite approvals in the v1 frontend (in the case of ChickenBonds).

This change makes it so infinite approvals stay infinite even after a transfer, which is also what OpenZeppelin does since some time ago:
OpenZeppelin/openzeppelin-contracts#3085

@danielattilasimon
Copy link
Collaborator Author

BTW any reason we can't just inherit OpenZeppelin's ERC20? Personally, I would feel more comfortable.

@bingen
Copy link
Collaborator

bingen commented Jun 19, 2024

BTW any reason we can't just inherit OpenZeppelin's ERC20? Personally, I would feel more comfortable.

I think it’s because we just renamed LUSD from v1 (and I don’t remember why we did that way then). But I agree, I would just import OpenZeppelin ECR20.

Copy link
Collaborator

@bingen bingen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (although again, I would import OZ ERC20 instead)

@danielattilasimon
Copy link
Collaborator Author

LGTM (although again, I would import OZ ERC20 instead)

I'll give it a go, but if it takes more than 20 minutes, I'll just merge this one and leave OZ for another time.

@bingen
Copy link
Collaborator

bingen commented Jun 19, 2024

I'll give it a go, but if it takes more than 20 minutes, I'll just merge this one and leave OZ for another time.

Perfect!! Thanks!

@danielattilasimon
Copy link
Collaborator Author

It turned out to be really easy.

@danielattilasimon danielattilasimon merged commit 2ced75a into main Jun 21, 2024
7 of 8 checks passed
@tomburgerch
Copy link
Collaborator

tomburgerch commented Jun 21, 2024 via email

@danielattilasimon danielattilasimon deleted the fix-infinite-approval branch July 3, 2024 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants